Migrate hinge angle injection to sensors simulator#2643
Conversation
GoogleCuttlefishTesterBot
removed
the
kokoro:force-run
| auto sensors_data = sensors_simulator_.GetSensorsData(mask); | ||
| std::lock_guard<std::mutex> lock(report_mtx_); | ||
| auto result = UpdateSensorsHal(sensors_data, data_channel_, mask); |
There was a problem hiding this comment.
I see that this was moved from data_reporter_thread_, but note abseil.io/tips/232
Use type deduction only if it makes the code clearer to readers who aren’t familiar with the project, or if it makes the code safer. Do not use it merely to avoid the inconvenience of writing an explicit type.
There was a problem hiding this comment.
Agreed with making sensors_data explicit (std::string) above, the type isn't obvious from GetSensorsData, but I kept result as auto since auto result = …; if (!result.ok()) for Result<void> is the idiom used throughout this file and the wider codebase. Happy to spell it out too if you'd rather go the other way.
There was a problem hiding this comment.
I think "idiom used throughout the wider codebase" still doesn't really satisfy the condition of "makes the code clearer to readers who aren't familiar with the project."
| element.dataset.adb = true; | ||
| } | ||
| } | ||
| element.dataset.adb = true; |
There was a problem hiding this comment.
Should this still be here? If I understand correctly, the code before amounted to something like "if the device state includes hinge_angle, make sure adb is connected."
As written this commit changes it to "always connect to adb," but I would expect "the hinge angle is not sent via adb" to imply "adb no longer matters for sensors, don't set or unset the adb connection requirement."
I may be misunderstanding what the .adb variable is for. FYI @jemoreira
There was a problem hiding this comment.
From the change perspective this makes sense, I'm agree.
However, after checking the getCustomDeviceStateButtonCb implementation, it looks to me like any action on that panel triggers adbShell('cmd device_state state reset'), meaning ADB is executed regardless of whether we change the hinge angle, the lid, or anything else, so I assume the entire panel should be disabled if ADB isn't available.
Do you think I'm missing something?
There was a problem hiding this comment.
Looks like adb should have been set regardless of device state including hinge angle or not.
The question that remains is: Is that device reset over adb still necessary if the hinge is handled via sensors?
There was a problem hiding this comment.
Looking at the change where this reset was introduced and another very old change. We used to have buttons triggering the following shell commands:
"shell_command":"dumpsys device_state | grep mCommittedState | grep OPENED && cmd device_state state 0 || cmd device_state state 2",
So device_state can be override via shell_command buttons and to ensure old "override" value isn't affecting the new-coming LID injection , this reset was introduced.
Checking the modern custom actions jsons across the internal tree, I cannot see anything similar and I think we should discourage using device_state this way and always go through the proper sensors.
Taken all of that I would prefer getting rid of this adb reset command (so adb dependency), but want to hear @jemoreira opinion after this context?
The cuttlefish_sensor_injection tool does not work with goldfish multihal sensors, meaning hinge angle changes currently have no effect To fix this, redirect WebRTC hinge angle updates to the sensors simulator instead. Test: hinge angle is updated from web ui Bug: 517370262 Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
| std::stringstream ss; | ||
| ss << x << sensors::INNER_DELIM << y << sensors::INNER_DELIM << z; | ||
| CF_EXPECT(SendCommand(sensors::kUpdateRotationVec, ss.str())); | ||
| UpdateSensorsUi(); |
There was a problem hiding this comment.
Similar to the discussion on on_change_callback_, what's the distinction that requires SetMotion calls UpdateSensorsUi but not SetHingeAngle?
| auto sensors_data = sensors_simulator_.GetSensorsData(mask); | ||
| std::lock_guard<std::mutex> lock(report_mtx_); | ||
| auto result = UpdateSensorsHal(sensors_data, data_channel_, mask); |
There was a problem hiding this comment.
I think "idiom used throughout the wider codebase" still doesn't really satisfy the condition of "makes the code clearer to readers who aren't familiar with the project."
4 participants
The cuttlefish_sensor_injection tool does not work with goldfish multihal sensors, meaning hinge angle changes currently have no effect
To fix this, redirect WebRTC hinge angle updates to the sensors simulator instead.
Test: hinge angle is updated from web ui
Bug: 517370262